Repository navigation
Conversation
|
|
|
Welcome @flrnd! |
|
/lgtm |
|
The same failure occurs on Java 11, 17, and 21 in the Windows Bazel build. In this case, I now wonder if it is worth finding a deterministic way to generate the timestamps, or if leaving the sleep in place makes sense. |
|
Another alternative to @@ -144,8 +138,8 @@ class EventCorrelatorTest {
Boolean expectedSkip)
throws Exception {
EventCorrelator correlator = new EventCorrelator();
+ OffsetDateTime now = OffsetDateTime.now();
for (CoreV1Event event : previousEvents) {
- OffsetDateTime now = OffsetDateTime.now();
event.setFirstTimestamp(now);
event.setLastTimestamp(now);
Optional<MutablePair<CoreV1Event, V1Patch>> result = correlator.correlate(event);
@@ -153,7 +147,7 @@ class EventCorrelatorTest {
correlator.updateState(event);
}
}
- OffsetDateTime now = OffsetDateTime.now();
+ now = now.plusNanos(10);
newEvent.setFirstTimestamp(now);
newEvent.setLastTimestamp(now); |
|
Feel free to try that alternate fix and update this PR. |
bd1ce4a to
9f9dcc7
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, flrnd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
There are now test failures in this test in some CI/CD runs. |
9f9dcc7 to
95b4da2
Compare
|
New changes are detected. LGTM label has been removed. |
|
Looking at both failing jobs, I think we're dealing with two separate timestamp issues. The first appears to have been addressed by increasing the timestamp increment to 1 ms, but the latest failures point to a second issue in Tracing the execution through
Update: Windows / JDK investigationAfter discussing this with folks on IRC and looking into the OpenJDK source, the issue seems more likely related to Windows clock resolution than to The investigation revealed a few interesting details:
This supports the hypothesis that the test relies on a clock-resolution guarantee that isn't available across all environments. It also explains why |
Replace Thread.sleep(100) with explicit timestamp increments in EventCorrelatorTest. Duplicate events preserve their input timestamps, so advancing those timestamps is sufficient to verify compression. Aggregated events, however, generate their own timestamps using OffsetDateTime.now(). Consecutive calls can return identical values depending on system clock resolution, causing intermittent CI failures on Windows. Following the approach used by the Go client, introduce an injectable timestamp source in EventAggregator using Supplier<OffsetDateTime>. Add a corresponding EventCorrelator constructor to pass the supplier through, while preserving the existing constructors and production behaviour with OffsetDateTime::now. Use an incrementing timestamp supplier in the test to control aggregation timestamps independently of the system clock. This preserves the existing timestamp assertions, removes the timing dependency, and avoids changes to EventLogger.
95b4da2 to
8c7c91f
Compare
Notes
Remove
Thread.sleep(100)fromEventCorrelatorTestand make timestamp generation deterministic without depending on elapsed wall-clock time.The investigation identified two timestamp paths: timestamps supplied by the test for ordinary duplicate events, and timestamps generated internally by
EventAggregatorfor aggregated events.CI failures
The initial change used
now.plusNanos(10)to separate the input timestamps. It passed on Linux and macOS across JDK 11, 17, and 21, and initially on Windows with JDK 11, but intermittent failures appeared on Windows with JDK 17 and 21. Later CI runs also failed on Windows with JDK 11 and 21, specifically in parameterized case[6](repeated aggregation of similar events).The failures were not consistently reproducible locally, including on a Windows 10 VM running Eclipse Temurin JDK 11, 17, and 21 with the same Bazel version and
--config ci.Investigation
Instrumenting
EventCorrelatorTestand tracing the flow throughEventCorrelator,EventAggregator, andEventLoggerrevealed two different behaviours:EventAggregatorreturns the original event.EventLoggerrestores the first observation'sfirstTimestampwhile preserving the incominglastTimestamp. Explicitly advancing the input timestamp makes these cases deterministic. Increasing the increment to 1 ms addressed the initial failures.EventAggregatorcreates a new event with both timestamps set from its ownOffsetDateTime.now()call. On repeated aggregation,EventLoggerrestores the previousfirstTimestampand retains the new aggregated event'slastTimestamp. If both clock reads return the same value, the assertion fails even though the event is correctly aggregated and counted.This explains why increasing the input timestamp alone did not fix case
[6]: the aggregator replaces those timestamps.Windows / JDK investigation
After inspecting the OpenJDK source, the failures appear consistent with coarse system-clock resolution on some Windows environments.
GetSystemTimeAsFileTime(). Its timestamps are represented in 100-nanosecond units, but that representation does not guarantee that successive calls produce distinct values.[6]assertion failure locally on Linux by temporarily truncating the aggregator'sOffsetDateTime.now()values to seconds.This reproduces the suspected failure mechanism, although the effective clock resolution on the CI runners was not measured directly. It also helps explain why
Thread.sleep(100)reduced the failures: it made separate clock readings more likely without guaranteeing them.Solution
Comparing the Java implementation with Kubernetes' Go client revealed that the Go correlator receives an injectable clock, and its tests use
SimpleIntervalClockto advance time predictably. The Java implementation follows similar aggregation logic but read the system clock directly inEventAggregator.The Java implementation now accepts a
Supplier<OffsetDateTime>for aggregation timestamps, while retaining the existing constructors and production behaviour. The default supplier is evaluated on each call:The test supplies an incrementing source instead:
EventCorrelatorpasses the supplier toEventAggregator;EventLoggerremains unchanged. The original timestamp assertions are preserved, and the test no longer relies on separate system-clock reads in the aggregation path.This follows the testability principle used by the Go client without introducing a custom
Clockclass or changing how production obtains time.Validation
EventCorrelatorTestpasses with the injected incrementing timestamp source.The remaining input-timestamp increment is intentional: ordinary duplicate events preserve the input timestamps, whereas aggregated events use the injected timestamp supplier.